Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use virtual environments for development #181

Open
wants to merge 26 commits into
base: master
Choose a base branch
from

Conversation

keggsmurph21
Copy link
Collaborator

@keggsmurph21 keggsmurph21 commented Oct 23, 2022

This PR cleans up a bunch of stuff, including

  • adding a new developer-facing dev/env.sh script (see the README)
  • removing the broken, unused ultratrace2 cruft
  • fixing the CircleCI build and running against Ubuntu 18.04, Fedora 35, and macOS Big Sur
  • removing unused packages
  • updating existing packages
  • updating documentation

Hopefully, this should make it easier to hack on this project now that everything can happen inside a virtual environment.

Unfortunately, I still can't help much with the folks working on Windows machines...

Instead of trying to support all possible platforms via an `install.sh`
script (which would need `root` privileges, in most cases), we should
just do a better job of documenting our system requirements.
We had this almost working, but, after `pip install`'ing our package,
it was crashing with

    $ ultratrace
    Traceback (most recent call last):
      File "/home/kevinmurphy/src/SwatPhonLab/UltraTrace/venv/ultratrace/bin/ultratrace", line 33, in <module>
        sys.exit(load_entry_point('ultratrace', 'console_scripts', 'ultratrace')())
      File "/home/kevinmurphy/src/SwatPhonLab/UltraTrace/venv/ultratrace/bin/ultratrace", line 25, in importlib_load_entry_point
        return next(matches).load()
      File "/home/kevinmurphy/src/SwatPhonLab/UltraTrace/venv/ultratrace/lib/python3.8/site-packages/importlib_metadata/__init__.py", line 169, in load
        return functools.reduce(getattr, attrs, module)
    AttributeError: module 'ultratrace.__main__' has no attribute 'main'
Right now, this is basically unsupported.  If we actually want to
build out some tools for managing test-data, we should probably
write something a bit more robust.
Instead of rewriting from scratch, it'll be a lot easier to just
tweak the existing system...
The `setup.cfg` should just declare "abstract" requirements
(which libraries do we need at runtime?).  The "concrete"
requirements (which version of each package do we need?)
should be declared in the `requirements.txt`.

See also: https://packaging.python.org/en/latest/discussions/install-requires-vs-requirements/
See also: https://caremad.io/posts/2013/07/setup-vs-requirement/
2. Make sure you have pip
3. Install ffmpeg and add to PATH
4. Run `setup.py`
TODO
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what else to say here.. I don't know how to install system packages on Windows. Maybe we should remove this section altogether? Once the system packages are installed, everything else should be the same...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not, because of non-Python library dependencies.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

non-Python library dependencies.

Yeah this is what I mean by "system packages".

We already directly depend on this package!
@keggsmurph21 keggsmurph21 changed the title Feature/install into virtual environment Use virtual environments for development Oct 23, 2022
@keggsmurph21 keggsmurph21 marked this pull request as ready for review October 23, 2022 03:03
@keggsmurph21
Copy link
Collaborator Author

cc @jonorthwash -- not sure if you want to take a look / give feedback or if you'd rather I just merge.

This was just from running

    $ black ultratrace/
AFAICT, we didn't actually need this for anything...

Closes #176
@jonorthwash
Copy link
Collaborator

The Phonetics Lab currently has three machines that run Debian (not to mention my office computer and personal machines). Could we make sure that's explicitly supported?


To lint/test `ultratrace`, use [`nox`](https://nox.thea.codes/en/stable/):
```sh
$ nox --help
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't say how to do anything specific.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, maybe it would be better to just say to run nox directly?

$ nox
nox > Running session install
nox > Creating virtual environment (virtualenv) using python in .nox/install
nox > python -m pip install --requirement ./requirements.txt
nox > Session install was successful.

There's more info if you follow that link to their docs, but basically running plain nox will run all of the tests defined here: https://github.com/SwatPhonLab/UltraTrace/blob/feature/install-into-virtual-environment/noxfile.py

black==22.10.0
mypy==0.982
nox==2022.8.7
pytest==7.1.3
Copy link
Collaborator

@jonorthwash jonorthwash Nov 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've never used most of these before. Why are they being introduced as dependencies?

pydub==0.25.1
pyparsing==3.0.9
python-dateutil==2.8.2
python-magic==0.4.27
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will break on Windows and probably macOS, and I wouldn't be surprised if locking it down to a specific version breaks on Linux in the future as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would pinning versions break? I think we're more likely to see breakages with unpinned versions..

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because not the same versions are available on all OSes of the packages that rely on compiled C libraries.

@jonorthwash
Copy link
Collaborator

Can we separate out some of these into different PRs? Like removing the ultratrace2 stuff I would merge right away, and I'd like to evaluate some of the other changes independently of one another. This is a bit much in one PR for me...

Copy link
Collaborator

@jonorthwash jonorthwash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments throughout.

@jonorthwash
Copy link
Collaborator

@keggsmurph21, I've tested this PR and it seems to work okay. But the conflicts need to be resolved before I can merge it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants